I recognize that an extra filestat is not the greatest for every file in the user's project. If there's somewhere higher level we can filter these out, I'm all ears.

@sectioneight a package declaration cannot contain ., it's not a valid character for an identifier, but a package's directory entry can contain a dot.

deadwood(~/src) % cat foo.go/foo.go 
package foo

const A = 1
deadwood(~/src) % cat bar.go 
package main

import (
        "fmt"

        "foo.go"
)

func main() {
        fmt.Println(foo.A)
}

ah perfect, i crossposted my review with the fix you just made 😄

Yes, I recognize this is a bit of an edge case, but it's one I ran into internally when trying to build an internal project with dep. Basically, we have some non-go stuff in a directory, and that directory contains a folder called ".go". This probably doesn't affect 99.99% of users but at least this way it ignores the "bad directories" instead of error-ing out with a cryptic message.

@davecheney are you saying that you'd like to modify this PR such that if we encounter a directory ("foo.go") we recurse into it?

@sectioneight sort of. I'm saying that the go tool does not treat directories that end in .go specially.

deadwood(~/src) % mkdir -p foo.go/bar.go/quxx.go
deadwood(~/src) % echo "package quxx" >> foo.go/bar.go/quxx.go/wargh.go
deadwood(~/src) % go list foo.go/...
foo.go
foo.go/bar.go/quxx.go

To be sure, it's a dumb idea, but not forbidden by the tool.

Ok, sounds like fillPackage is in need of a refactor. Should I abandon this PR in favor of another approach? It seems we'll need to change the delineation of fillPackage and ListPackages

guys guys, i addressed this in my earlier review comments 😄

skipping a dir named foo.go in fillPackages() has no bearing on whether we later walk into foo.go.

Aha, excellent point :)

Though, given the discussion here, it probably would be worth adding some real .go files under the foo.go fixture dir to provide a check that we're respecting that.

Any concerns about the os.Stat calls for every source file? This is my first contribution to the project so I'm not aware of what the benchmarks/requirements are as far as introducing stuff that could "make dep slower"

@sdboyer sounds good, will do

This is my first contribution to the project

🎉 🎉 🎊 🕺 🎉 😄

Any concerns about the os.Stat calls for every source file?

Yes, it is a bit concerning; this is a hotpath, and we've already got #419 open to swap in a faster alternative to filepath.Walk() that gets a lot of its speedups from avoiding unnecessary stat calls.

However, it's fine for the purposes of this PR, as I think the best fix for it here may be, in #419, to have the replacement walker be responsible for injecting the results of a platform-sensitive, possibly-faster readDir() into the walk func. That would carry file vs. dir info, obviating the need for the Stat you've introduced.

So, I'm fine with it, as it brings us closer to existing tool behavior, and we have a concrete path to removing the performance regression.

/cc @jstemmer - fyi, since you're looking at #419, here's a bit of an evolved requirement 😄

just a license header, now

Ok, I think we're all good now, performance considerations aside. Let me know if that's something I can help with -- I'm interested in moving my company to dep as soon as is reasonable :)

looks good, thank you! and yayyyy first contribution! 🎉

@sectioneight also, with any luck, it'll be reasonable to begin exploring moving your company to dep in earnest on Monday, once we stabilize the manifest and lock. 😄

re: things you could help with; TBH, i've been kicking the performance can down the road for a while. #67 lays out my basic mental roadmap for it, and why I've been vaguely OK with doing so (basically, there's a ton of headroom and a clear path to improvement, albeit with some now-mostly-cleared blockers). #289 is the meta issue for performance, and #431 is now the big ticket item there.

Not in the meta-issue, though, is establishing some broader benchmarks that might help us to identify performance regressions. If you're interested in that sort of thing, that'd be cool - open up an issue and we can brainstorm about what that might look like 😄

On a different front, I haven't forgotten about the use case you raised last year in Masterminds/glide#372. I'm still finding it to be a problem for the same basic reason - how arbitrary changes to source URLs affect generated file portability, and how we cope with that - though it's been discussed in somewhat greater detail in #174. At this point, I'm inclined to say that we might be best off having a toggle that basically causes the system to reach out to a go get metadata-like service the first time (per run) it encounters every URL in order to get a rewritten one. If you wanted to work on hammering something like that out, it'd also be super helpful 🎉

Ya, one of the reasons I wanted my team to get involved early is to make sure we can come up with an amenable "rewrite" / airgap solution that works for us as well as the community. I'm going to pick off a few smaller tasks and then will see if I can look into perf.

Also big +1 on your idea, it would be trivial to stand up a metadata service internally and distribute a file to engineers computers in a well-known location that points to said server, where we can have control over rewrites rather than ending up in massive TOML files

FYI I opened golang/go#20337 for clarity on what the correct behavior should be for scanning directories. As far as I can tell this is the first tool (that I've used, at least) that ignores imports starting with "." or "_", and the documentation is unclear since it only talks about files. Depending on the response there we may want to rejigger the code so that it doesn't ignore dot-dirs: